[4.0] Deprecate Pathway::getInstance #19528
Conversation
|
I'm unsure about this - the pathway is constant for a given page - we shouldn't load this multiple times. If we aren't doing a |
|
True, was a bit in a rush and didn't saw that it caches the instances per client. Sounds like another factory in the container. Do we want to go that way? |
|
I think just do it like the application in the container yeah? This ones kinda weird because there isn't an Admin Pathway (at least at the moment) - although with the admin menu changes in 3.8 it wouldn't totally surprise me if one came along at some point |
|
Modified the pr to get the pathway object from the container by a service provider. Changed the deprecate message to get the pathway from the app, as every core extension does already. |
|
A little late to the party here, but... Shouldn't there just be a |
| */ | ||
| public function register(Container $container) | ||
| { | ||
| $container->alias('SitePathway', SitePathway::class) |
There was a problem hiding this comment.
Nitpick. IMO container keys should either be class names or some kind of dot separated key like you use for Registry options. So this particular service should have one key and two aliases:
SitePathway::class(already in place)JPathwaySite(legacy class name)pathway.site(this is a pathway service and the second part of the key specifies a specific sub-object, this also does not block potential future use of apathwaykey if we have other apps supporting one and in the future if we had it we'd add apathway.administratorservice).
|
Added your input. |
4 participants
Ongoing effort to phase out getInstance code. This pr deprecates Pathway::getInstance.
@wilsonge and @mbabker can the code which gets the pathway from the factory being removed here?
I'm splitting #16918 into different pr's to be easier to review.